Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support MySQL GRANT SENSITIVE_VARIABLES_OBSERVER #4412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kgalieva
Copy link

Adding support for GRANT SENSITIVE_VARIABLES_OBSERVER for MySQL
https://dev.mysql.com/doc/refman/8.4/en/privileges-provided.html

@kaby76
Copy link
Contributor

kaby76 commented Feb 12, 2025

@mike-lischke Is this change correct to the Oracle grammar? I just updated the grammar to the lastest from the vscode extension. #4408

@mike-lischke
Copy link
Member

mike-lischke commented Feb 13, 2025

This PR should be rejected. It adds a dynamic privilege to the grammar. The grant statement not only allows to specify predefined privileges, but also simple identifiers. Hence you can use input like grant SENSITIVE_VARIABLES_OBSERVER to [email protected] and does a runtime lookup for the given priv name.

The upstream fix is also wrong btw. @kgalieva. And, very important, syntax which is supported only starting at a specific MySQL version should be guarded using a semantic predicate. You can see that everywhere in the MySQL grammar.

@mike-lischke
Copy link
Member

Side note: this PR is not for the Oracle grammar, but the one from Positive Technologies. I'm not sure this repository should keep multiple MySQL grammars.

@kaby76
Copy link
Contributor

kaby76 commented Feb 13, 2025

Side note: this PR is not for the Oracle grammar, but the one from Positive Technologies. I'm not sure this repository should keep multiple MySQL grammars.

Sorry, I see that now. I only saw that a test was modified over in the Oracle grammar, and it parses fine.

$ ./bin/Debug/net8.0/Test.exe -input 'GRANT SENSITIVE_VARIABLES_OBSERVER ON *.* TO `admin`@`%`;' -tree
(queries (query (simpleStatement (accountManagementStatement (grantStatement GRANT (roleOrPrivilegesList (roleOrPrivilege (roleIdentifierOrText (roleIdentifier (pureIdentifier SENSITIVE_VARIABLES_OBSERVER))))) ON (grantIdentifier * . *) TO (grantTargetList (userList (user (userIdentifierOrText (textOrIdentifier (identifier (pureIdentifier `admin`))) (userVariable @ (textOrIdentifier (identifier (pureIdentifier `%`))))))))))) ;) <EOF>)
CSharp 0 string0 success 0.103722
Total Time: 0.2760364
02/13-06:20:05 ~/issues/g4-current/sql/mysql/Oracle/Generated-CSharp

I agree. It might be time to archive the PT grammar, or just remove it.

@mike-lischke
Copy link
Member

@teverett @KvanTTT What do you think? Shall we remove the PT MySQL grammar, in favor of the Oracle grammar? I know the PT grammar also supports MySQL 5.6 and 5.7, but these versions are EOL already and the PT grammar is only partially correct. If people really want support for these versions, they can download an earlier version of the current Oracle grammar from https://github.com/mysql/mysql-workbench/blob/8.0/library/parsers/grammars/.

@KvanTTT
Copy link
Member

KvanTTT commented Feb 15, 2025

I'm not sure, because other users still use these grammars and even make pull requests. For instance, the latest was on Jan 16: #4383

Please ask them at first (@ani-sha) and create a related topic to make it noticable.

@KvanTTT KvanTTT added the mysql-PositiveTechnologies MySql grammar by Positive Technologies issues label Feb 15, 2025
@mike-lischke
Copy link
Member

I have a conflict of interests here, as my grammar would prevail, so maybe someone else is the better choice to drive this process. This PR here as well as the one you mentioned @KvanTTT are both wrong (I explained that above).

All I can do here is to suggest to remove the PT grammar, because it is not correct. It doesn't matter that someone still files PRs for it (it shows only the grammar needs changes to be correct). What happens instead it that PRs like this and #4383 add things that don't belong in this grammar. It's a MySQL grammar, not a TokuDB or Azure grammar. So with every such PR the grammar gets more and more incorrect, not to mention that it is way behind the actual MySQL time line (we are already working on 9.x).

@teverett
Copy link
Member

I have no concerns with keeping the PT grammar. If it's wrong, perhaps it could be corrected?

@KvanTTT
Copy link
Member

KvanTTT commented Feb 16, 2025

It doesn't matter that someone still files PRs for it (it shows only the grammar needs changes to be correct).

Well, but I think it matters.

I have no objection regarding removing, but please create an issue regarding the topic to notify users explicetly and get more other opinions. Unfortunately, legacy is not fun, but it's an important part of community support.

@ani-sha
Copy link
Contributor

ani-sha commented Feb 17, 2025

Hi @KvanTTT, thanks for tagging me in this discussion. I would like to highlight that Debezium has been using the Positive Technologies MySQL grammar since at least 2017. To keep up with the evolving MySQL syntax, our maintainers (and community members) actively contribute updates to the upstream PT grammar whenever we make any changes in our grammar.

It doesn't matter that someone still files PRs for it (it shows only the grammar needs changes to be correct). What happens instead it that PRs like this and #4383 add things that don't belong in this grammar. It's a MySQL grammar, not a TokuDB or Azure grammar. So with every such PR the grammar gets more and more incorrect.

In the past, we haven’t received feedback on our PRs regarding these concerns. We didn’t anticipate that the TokuDB or Azure grammar PRs would be merged here, but since they were, we contributed to ensure support for all MySQL grammar variations. That said, we are open to refining our contributions and providing specific semantic predicates where needed.

Please create an issue regarding the topic to notify users explicetly and get more other opinions.

That would be helpful. I will need to discuss these changes with my team.

@mike-lischke
Copy link
Member

Please note: it was not my intention to use this issue to decide about the future of the PT grammar. All I wanted was to get a general yes/no input from important people in this process.

Of course there must be a good deprecation path etc. to phase out the grammar. Ideally one of the owners of it should take the stab and start this process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mysql-PositiveTechnologies MySql grammar by Positive Technologies issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants